Replace IngredientCache lock with atomic primitive#687
Replace IngredientCache lock with atomic primitive#687Veykril merged 1 commit intosalsa-rs:masterfrom
IngredientCache lock with atomic primitive#687Conversation
✅ Deploy Preview for salsa-rs canceled.
|
| assert!(v < (u32::MAX as usize)); | ||
| assert!(v <= u32::MAX as usize); |
There was a problem hiding this comment.
I believe both of these checks (here and for MemoIngredientIndex) were accidentally wrong
CodSpeed Performance ReportMerging #687 will not alter performanceComparing Summary
|
Veykril
force-pushed
the
veykril/push-pwqsvzurlkun
branch
from
4316b76 to
a93ba43
Compare
Veykril
force-pushed
the
veykril/push-pwqsvzurlkun
branch
from
a93ba43 to
d894400
Compare
src/zalsa.rs
Outdated
| _ = self.cached_data.compare_exchange( | ||
| Self::UNINITIALIZED, | ||
| packed, | ||
| Ordering::AcqRel, |
There was a problem hiding this comment.
Release is sufficient here, there's no need for Acquire if the current thread wins the race (though of course both are correct).
Veykril
force-pushed
the
veykril/push-pwqsvzurlkun
branch
from
d894400 to
971e52b
Compare
src/zalsa.rs
Outdated
| // This is a packed representation of `Option<(Nonce<StorageNonce>, IngredientIndex)>` | ||
| // allowing us to avoid a lock in favor of an atomic load. This works thanks to `Nonce` | ||
| // having a niche and so the entire type can fit into a u64. |
There was a problem hiding this comment.
| // This is a packed representation of `Option<(Nonce<StorageNonce>, IngredientIndex)>` | |
| // allowing us to avoid a lock in favor of an atomic load. This works thanks to `Nonce` | |
| // having a niche and so the entire type can fit into a u64. | |
| // A packed representation of `Option<(Nonce<StorageNonce>, IngredientIndex)>`. | |
| // | |
| // This allows us to replace a lock in favor of an atomic load. This works thanks to `Nonce` | |
| // having a niche, which means so the entire type can fit into an `AtomicU64`. |
davidbarsky
left a comment
davidbarsky
left a comment
There was a problem hiding this comment.
i think this looks good to me? the only note that i'd have is "it'd be nice to have a Loom/shuttle test for IngredientCache, but I think I committed to that...
Veykril
force-pushed
the
veykril/push-pwqsvzurlkun
branch
2 times, most recently
from
005375e to
3396a1a
Compare
|
Ye I'd push the test onto the rest of the Loom work. I don't think without Loom writing a test is too useful here right now Though we can also delay this until we have loom integrated, no preference from me. |
Veykril
force-pushed
the
veykril/push-pwqsvzurlkun
branch
from
3396a1a to
f2b1517
Compare
|
I think we can/should merge this, but codespeed is reporting 1–2% regressions. do you happen to why they're showing up? |
|
I don't know why, I'd expect this to not regress it. Maybe its just within noise |
|
1-2% is something I consider noise. |
Veykril
force-pushed
the
veykril/push-pwqsvzurlkun
branch
from
f2b1517 to
6181ddf
Compare
5 participants
No description provided.